Repository navigation
Fix #15077 FP uninitvar for array of std::vector - #8908
chrchr-github wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| setTokenValue(tok, std::move(value), settings); | ||
| } else if (tok->variable() && tok->variable()->isArray() && !tok->variable()->isArgument() && | ||
| tok->variable()->getTypeName() != "std::array") { | ||
| !tok->variable()->isStlType("array")) { |
There was a problem hiding this comment.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve
isStlType("array") is not exactly equivalent to the old getTypeName() == "std::array". It only checks typeStartToken()->strAt(2), so nested types such as std::array<int,3>::value_type are now treated as std::array too. I compared main and this PR:
void h1() { std::array<int,3>::value_type a[2] = {}; if (a) {} }
void h2() { std::array<int,3>::value_type a[2] = {}; if (a == nullptr) {} }main: knownConditionTrueFalse on both (correct, it is a plain int[2]).
PR: no knownConditionTrueFalse, but instead a new FP nullPointerRedundantCheck: Either the condition 'a' is redundant or there is possible null pointer dereference on both lines.
The same applies to valueFlowArrayBool() (line 730). It is admittedly an unusual way to write code, and the warnings on lib/, cli/, test/cfg/ and samples/ are identical, so it is minor. But it could be avoided by keeping getTypeName() == "std::array" in the places that only did a rename, or by making Variable::isStlType(const std::string&) reject a type that continues with :: after the template arguments.
There was a problem hiding this comment.
@danmar: Do you see what's happening here? First, there is a suggestion, and only after adopting it I'm told it's a bad idea. What a waste of time.
There was a problem hiding this comment.
yeah I am sorry about this! this went terribly wrong but I feel that there has been overall mostly good reviews from AI lately?
I have asked claude to verify findings.
|
|
||
| const Variable *var = varTok->variable(); | ||
| if (var && var->isArray() && var->isArgument() && !var->isReference() && !(var->isStlType() && var->getTypeName() == "std::array")) | ||
| if (var && var->isArray() && var->isArgument() && !var->isReference() && !var->isStlType("array")) |
There was a problem hiding this comment.
This is an AI review take it with a grain of salt. Please feel free to reject it by clicking on Resolve
Same isStlType("array") vs getTypeName() == "std::array" difference as in valueflow.cpp. This one causes a false negative:
int g6(std::array<int,3>::value_type a[2]) { return sizeof(a); }main warns sizeofwithsilentarraypointer, the PR is silent. (The old code here had both isStlType() and getTypeName() == "std::array", so it was exact.)
|
Test results for commit 27c4120 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8908/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
|
This pull request has merge conflicts that must be resolved. |
|
Test results for commit 275c775 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8908/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
275c775 to
d112a5f
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems. The PR fixes the uninitvar false positive for an array of std::vector. In needsInitialization() the old check (isStlType() && isArray()) also matched C arrays of any std container; the new check matches only std::array. The isStlType("array") versions of the code that earlier reviews commented on are no longer in the diff. All the std::array warnings that I tested are unchanged.
The checkMemsetType refactoring changes one thing that users can see: for a nested std type, the type named in the memsetClass/mallocOnClassError message is now longer. std::vector<int>::iterator used to be reported as 'std::vector' and is now 'std::vector::iterator'. This is arguably more accurate, and no test covers it. The podtype() lookup now also gets the full nested name, so it can only matter if a library configuration defines nested podtypes.
Testing
- I built
cppcheckandtestrunnerinpr/andcppcheckinbase/. Both builds succeeded. ./testrunner TestUninitVar TestClass TestValueFlow: 0 tests failed.- I compared base and PR with
cppcheck -q --enable=all --inconclusive --library=stdon my own test files:- Arrays of containers: for
const std::vector<int> a[1]; return a[0];(#15077) base reportsuninitvarand the PR reports nothing.std::string a[2],std::wstring a[2]andstd::vector<int> a[2][2]give no warning in either version. std::array:std::array<int,3> a;,std::array<int,3> a[2](used asa[0][0]and asa[1]),std::array<POD,3>andusing A = std::array<int,3>(bothA aandA a[2]) still reportuninitvarin the PR, the same as base.std::array<std::vector<int>,3>gives no warning in either version.::std::array<int,3>: neither version warns. This is an existing gap, not something the PR introduced.- memset on a struct with members
std::vector<int>::iterator it; std::string s;: both versions reportmemsetClass. The message changed fromcontains a 'std::vector'tocontains a 'std::vector::iterator'.
- Arrays of containers: for
Automated review of d112a5f by Claude. The pull request was built and tested locally.
No description provided.